Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Go to use net.FD for peer and host #725

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Apr 8, 2024

Remove all host, peer parsing for Go in eBPF, rely on getting the information consistently from the connection information.

@grcevski grcevski requested review from mariomac and marctc as code owners April 8, 2024 21:38
Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines +6 to +7
#define MAX_CONCURRENT_REQUESTS 10000 // 10000 requests per second max for a single traced process
#define MAX_CONCURRENT_SHARED_REQUESTS 30000 // 10 * MAX_CONCURRENT_REQUESTS total ongoing requests, for maps shared among multiple tracers, e.g. pinned maps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could address this as another task, for other maps too: you can move the map sizing from compile-time to runtime and let the user decide the size (this way we could troubleshoot better).

This is how the Network Metrics side does it at the user side, from the user configuration:

https://github.com/grafana/beyla/blob/main/pkg/internal/netolly/ebpf/tracer.go#L85-L86

The aggregated_flows map does not define any size when it is compiled:

https://github.com/grafana/beyla/blob/main/bpf/flows.c#L56-L60

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great point, I'll follow-up with a configuration option. 1000 was way too small, too many collisions on my 20 core machine.

@grcevski grcevski merged commit 3f5cc63 into grafana:main Apr 11, 2024
4 checks passed
@grcevski grcevski deleted the change_go_ip_report branch April 11, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants